Skip to content

feat: Replace LinkedList with ArrayList #3766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Feb 24, 2025
Merged

feat: Replace LinkedList with ArrayList #3766

merged 4 commits into from
Feb 24, 2025

Conversation

@janeklb
Copy link
Contributor

@janeklb janeklb commented Feb 24, 2025 •
edited
Loading

Initialise the "batch of records" list container as an ArrayList rather than LinkedList.

Using a LinkedList has an impact on performance as outlined here #3764

(fixes #3764)

artembilan
artembilan requested changes Feb 24, 2025
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got a failed test with your change:

 SubBatchPerPartitionTests > withFilter() FAILED
    org.opentest4j.AssertionFailedError: 
    Expecting value to be true but was false
        at app//org.springframework.kafka.listener.SubBatchPerPartitionTests.withFilter(SubBatchPerPartitionTests.java:121)

And you indeed mentioned that in the issue.
WDYT?

@janeklb
Copy link
Contributor Author

janeklb commented Feb 24, 2025

We got a failed test with your change:

 SubBatchPerPartitionTests > withFilter() FAILED
    org.opentest4j.AssertionFailedError: 
    Expecting value to be true but was false
        at app//org.springframework.kafka.listener.SubBatchPerPartitionTests.withFilter(SubBatchPerPartitionTests.java:121)

And you indeed mentioned that in the issue. WDYT?

Apologies -- I raised the PR too quickly. I assumed using Arrays.asList would have fixed the failing test (because List.of produces an immutable list and the failure seemed to be related to mutation..), but I only re-ran the tests locally after pushing this PR. Will investigate and let you know shortly.

@janeklb
Copy link
Contributor Author

janeklb commented Feb 24, 2025 •
edited
Loading

Turns out my comment here #3764 (comment) was totally wrong. I misunderstood the implementation of ArrayList#add(E element). Your suggestion @artembilan #3764 (comment) #3764 (comment) works perfect 👍🏻

artembilan
artembilan approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Will be merged (and back-ported) when all tests pass.

Thank you for your contribution!

@janeklb
Copy link
Contributor Author

janeklb commented Feb 24, 2025

LGTM. Will be merged (and back-ported) when all tests pass.

Thank you for your contribution!

Wonderful, thank you for helping me getting over the line. When should I expect a new 3.3.x release?

@artembilan artembilan enabled auto-merge (squash) February 24, 2025 20:09
@artembilan
Copy link
Member

When should I expect a new 3.3.x release?

See our Release Calendar for March: https://spring.io/projects#release-calendar

@artembilan artembilan merged commit 53149d4 into spring-projects:main Feb 24, 2025
3 checks passed
@janeklb janeklb deleted the jlb/array-list-poc branch February 24, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artembilan artembilan artembilan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve performance of acknowledge(int index) / override createRecordList implementation

2 participants

@janeklb @artembilan